Fix ellipseMode(CORNERS) and rectMode(CORNER) #7290
Conversation
|
🎉 Thanks for opening this pull request! Please check out our contributing guidelines if you haven't already. And be sure to add yourself to the list of contributors on the readme page! |
|
Hi, Thanks for taking up this task. I believe I have tested this and it works well. I have a concern: are you planning to write tests for it in the If so, I don't think that approach would work for your case. Perhaps we could write unit or visual tests instead. What's your opinion? |
modeAdjust() now always returns the top-left corner and a positive width and height.
|
Hi, I've added proper visual test and removed |
@all-contributors please add @martinleopold for bug, code |
|
I've put up a pull request to add @martinleopold! 🎉 |
|
|
||
| for (let mode of MODES) { | ||
| visualTest(`Mode ${mode}`, function(p5, screenshot) { | ||
| p5.createCanvas(240, 500); |
There was a problem hiding this comment.
Alright, I believe creating a larger canvas might be an issue. When the tests run, a bigger canvas could make them take a bit longer due to its size. Plus, including files that are 15-20kB could make the codebase pretty heavy. Could we perhaps reduce it to 50x50? That would probably take up minimal space in bytes.
By doing this, we could include the tests one by one. Like, for each quadrant we can add one tests or something like that? For reference, you might want to check out this link: https://github.com/processing/p5.js/pull/6783/files#diff-aefa4b8e5287fbd0cdf37a6ec8d27322a4e5c893e99ec74d32053e60e2bdfd7d
Quadrant-1 -> all modes -> 1 particular tests? Or do you have any idea how could 50X50 size canvas could be fit for testing? I am also not 100% sure.
Maybe by using a 50x50 size, we could adjust the parameter values of _shapeCorners accordingly?
There was a problem hiding this comment.
I think in this case using many smaller canvases instead of a few bigger ones, will actually make things slower. Because then we'll have the overhead of createCanvas() i.e. creating a rendering context much more often, and the overall codebase size will also be larger with many small PNGs because of the PNG headers.
And to thoroughly test everything, we have 4 modes, times 3 shapes, times 4 quadrants, times 4 combinations of points, equals 192 shapes to draw. I see no way around that. Bear in mind this adds 192 test for fundamental drawing operations that were previously untested.
Therefore I propose making the canvas in the tests as small as possible, while still testing all the cases.
I have reduced the canvas size from 240x500 to 60x125 px, which reduces the total size of the generated PNGs from 223 to 12 kB.
From 240x500 to 60x125 px. PNG size from 223 to 12 kB.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Looks good to me.
Requesting @davepagurek for a round of review since I haven't used ellipseMode() much, so needs another round of review. Also, very thanks for your patience @martinleopold I am really sorry for the delay on reviewing this PR, also thanks for your hardwork.
Resolves #7289
Changes:
This mainly resolves a rendering/crashing issues with
ellipseMode(CORNERS)affectingellipse()andarc(); see issue above). It also fixes a related issue with usingrectMode(CORNER)and negative widths and/or heights.The bulk of the changes are in
modeAdjust()which should now properly handle the modes and return a normalized bounding box. As a result a few lines of handling negative widths and heights needed to be removed fromarc()and_renderEllipse().I've included a test sketch in test/shape-modes.html where you can cycle through all the different modes and shapes. I'm happy to update this, if somebody could direct me on how to properly test this case. I'm not sure, since it's mostly about the rendering output.
Screenshots of the change:
PR Checklist
npm run lintpasses